-
Notifications
You must be signed in to change notification settings - Fork 32
#235 [Coding Guideline]: Do not create values from uninitialized memory #240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I've made some improvement suggestions in a PR here: manhatsu#1 |
Thank you very much. Merged to this branch |
|
Hey @manhatsu 👋 it looks like from the CI that a new tag needs to be added. Could you follow what @rcseacord did in this PR to add the |
d3b29b5 to
57f303a
Compare
57f303a to
4b8cbc7
Compare
PLeVasseur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @manhatsu -- thank you for contributing. Please see the comment I left on how to generate a template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What @inkreasing says is correct. This description is insufficient to reflect the restrictions imposed by MIRI here:
Note this is not UB before line 15.
Bytes remain uninit until written. You may not read uninitialized bytes as any initialized type, period, not even if "all" bitpatterns are considered valid, because uninit is the 257th bitpattern for a byte, effectively: 0xUU. By contrast, u8 is 0x00 through 0xFF, inclusive. We use MaybeUninit<u8> to indicate the final state is possible, and it is valid to read that value (well, from any allocation that has a byte in it, at least).
src/coding-guidelines/values.rst
Outdated
| A program shall not create a value of any type from uninitialized memory, | ||
| except when accessing a field of a union type, | ||
| where such reads are explicitly defined to be permitted even if the bytes of that field are uninitialized. | ||
| It is prohibited to interpret uninitialized memory as a value of any Rust type such as a | ||
| primitive, aggregate, reference, pointer, struct, enum, array, or tuple. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definition does not consider composition of types:
use std::mem::MaybeUninit;
union Uninit32 {
u: u32,
i: i32,
f: f32,
void: (),
}
struct Newtype32(Uninit32);
fn main() {
let x: Newtype32 = unsafe { MaybeUninit::uninit().assume_init() };
}This passes miri because all the bytes in Newtype32 are defined by Uninit32, which is allowed to be uninitialized.
When bytes are read as a type (e.g. by using ptr.read(), *ptr, or mem::transmute), a "typed read" occurs. This asserts the bytes are valid as that type. Uninit32 and MaybeUninit are the same thing here: unions with () as a possibility, which means they must be valid to read from a blob of uninitialized bytes within a valid allocation1. Because Newtype32 is entirely defined by Uninit32, it is also valid to read from uninitialized bytes: the struct wrapper does not impose a novel validity requirement. If this is a mandatory guideline, it should be more exacting about why.
Footnotes
-
Note that this is likely a stronger requirement than the actual rules will be regarding union validity once final details of those are hashed out. I'm just giving an example that is very "in the clear". ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
| .. non_compliant_example:: | ||
| :id: non_compl_ex_Qb5GqYTP6db1 | ||
| :status: draft | ||
|
|
||
| This noncompliant example creates a value of type ``u32`` from uninitialized memory via | ||
| `assume_init <https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.assume_init>`_: | ||
|
|
||
| .. code-block:: rust | ||
|
|
||
| use std::mem::MaybeUninit; | ||
|
|
||
| let x: u32 = unsafe { MaybeUninit::uninit().assume_init() }; // UB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got this right, but the lesson needs to be extended elsewhere: assume_init and the read of a union field at its type are not really different operations in the semantics, so why would u32 be valid to read from a union field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
9b81ff4 to
0e2776c
Compare
felix91gr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this helps
workingjubilee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better.
|
Sidenote: this probably should be part of the Unsafety chapter. Anything dealing with upholding validity invariants in |
I created an issue #241 that discusses this. See what you think. Anyway, we should probably have this discussion there. |
dcb19bf to
8d4a655
Compare
|
Hi @manhatsu, @rcseacord -- please see this PR: #288 Please simply replace the current commits on your feature branch with that single commit on the above PR. That way we can keep the review history on this PR. |
|
@felix91gr I might be coming around to your view that union should be split out into a different rule. |
20d6c35 to
2ade1b3
Compare
| or related functions, is treated in the same manner as a typed read. | ||
| Calling these function when on memory that is not yet fully initialized causes immediate undefined behavior. | ||
| The memory must be properly initialized according to the requirements of the variable’s type. | ||
| For example, a variable of reference type must be aligned and non-null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if you want to fully list all validity requirements here, but references also need to point to valid memory.
This code has UB:
fn main() {
let mut uninit: MaybeUninit<&u8> = MaybeUninit::uninit();
unsafe {
// write non-null and aligned address.
(&raw mut uninit).cast::<*const u8>().write(ptr::dangling());
// UB here
let init = uninit.assume_init();
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this requirement and created a new noncompliant example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
rcseacord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
deunionfied
had a slightly older version, so I replaced it with the latest.
added another example and some clarification for references
spelling
Co-authored-by: increasing <[email protected]>
Clarified guideline on reading uninitialized memory, specifying non-union types and adding citations for better understanding.
Add miri directives and normalize citations/bibliography entries so the guideline builds cleanly.
802b444 to
6ce3ddb
Compare
Allow warnings for UB demonstrations and prefix unused locals in the uninitialized-memory guideline to keep example tests clean.
| :id: non_compl_ex_Qb5GqYTP6db3 | ||
| :status: draft | ||
|
|
||
| This noncompliant example creates a reference from uninitialized memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This noncompliant example creates a reference from uninitialized memory. | |
| This noncompliant example creates a reference without provenance. |
Or maybe something that the reference is not dereferencable. (because the memory it points to doesn't exist)
|
|
||
| The variable ``buf`` is a fully, zero-initialized 8-byte buffer. | ||
|
|
||
| The first wo bytes of ``buf`` are overwritten. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The first wo bytes of ``buf`` are overwritten. | |
| The first two bytes of ``buf`` are overwritten. |
Closes #235.